feat(impact): ship deterministic commandf impact - #27
Conversation
|
Warning Review limit reachedNext included review available in 38 seconds. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
ⓘ Your Qodo trial ends soon. Ask your workspace admin to set up billing to keep reviews running after the trial. Manage billing |
|
@coderabbitai review |
|
/review |
|
|
@coderabbitai review |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
PR Summary by QodoAdd deterministic commandf impact CLI and proof
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. Ambiguous package error untested
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if matches.next().is_some() { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| format!("package {package_name} appears more than once in the lockfile"), |
There was a problem hiding this comment.
1. Ambiguous package error untested 📘 Rule violation ▣ Testability
The new duplicate-package rejection in select_locked_package has no automated test that constructs an ambiguous lockfile and asserts the CLI error behavior. This leaves a distinct business-logic conflict branch unverified.
Agent Prompt
## Issue description
Add deterministic automated coverage for the `commandf impact` branch that rejects duplicate entries for the selected package.
## Issue Context
Construct a schema-v2 lockfile containing the requested package more than once, invoke the shipped CLI, and assert the nonzero exit status, empty stdout, and exact duplicate-package diagnostic.
## Fix Focus Areas
- crates/commandf-cli/src/impact.rs[83-87]
- crates/commandf-cli/tests/impact_behavior.rs[73-107]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| before_cache.verify(&before_locked.sha256)?; | ||
| after_cache.verify(&after_locked.sha256)?; | ||
|
|
||
| let before_bytes = read_locked_archive(&before_cache, before_locked)?; |
There was a problem hiding this comment.
2. Cache verification race 🐞 Bug ☼ Reliability
The selected archive is verified, discarded, and then reopened with plain fs::read, so a cache object changed between those operations can supply bytes that do not match the lock digest while the diff still labels them with that digest. This breaks the verified-cache trust boundary and can produce an internally inconsistent impact report.
Agent Prompt
## Issue description
The impact CLI verifies a cache object and then rereads it outside `PackageCache`'s digest-checking boundary, allowing a check/use race and performing redundant I/O.
## Issue Context
`PackageCache::read_verified` returns the exact bytes whose digest it checked. Keep those bytes and pass them to the diff instead of calling `verify` followed by `fs::read`.
## Fix Focus Areas
- crates/commandf-cli/src/impact.rs[27-31]
- crates/commandf-cli/src/impact.rs[60-67]
- crates/commandf-pkg/src/cache.rs[63-85]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| let before_lockfile = Lockfile::from_slice(&fs::read(before_lock)?)?; | ||
| let after_lockfile = Lockfile::from_slice(&fs::read(after_lock)?)?; |
There was a problem hiding this comment.
3. Inputs bypass size bounds 🐞 Bug ☼ Reliability
impact::run uses unrestricted fs::read for user-supplied lockfiles and cache archives, allowing oversized local inputs to allocate memory before parsing or bounded archive scanning can reject them. The archive scanner's 512 MiB limit applies only to decompressed traversal after the complete compressed archive is already resident in memory.
Agent Prompt
## Issue description
The new CLI reads arbitrary lock and cache files fully into memory before enforcing any input-size or decompression-work limits.
## Issue Context
Add explicit persisted-lock and compressed-archive byte limits at the read boundary, rejecting oversized files before allocation. Preserve the existing decompressed-byte, entry-count, and resource-size checks.
## Fix Focus Areas
- crates/commandf-cli/src/impact.rs[18-19]
- crates/commandf-cli/src/impact.rs[27-31]
- crates/commandf-pkg/src/cache.rs[67-85]
- crates/commandf-pkg/src/artifact_scan.rs[9-11]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Stack
CF-12 implementation Stack B for tasks T020-T025.
Base: canonical Stack A merge
d46591f0f7224d49fda0d89a6a79cc418fba534e.Summary
Ship the user-visible
commandf impactCLI as a thin adapter over the canonical CF-12 library contract and add dedicated deterministic proof.Changes
commandf impact <package>with explicit before/after lock/cache inputs and JSON-only V1 output;build_impact_report;cf12-impact-proofusing the immutable Rust 1.97.1 container and pinned actions;CF12_IMPACT_SHA256=<sha256>and retain the checksum as a short-lived workflow artifact;Explicit non-goals
Keep Draft until all applicable exact-head workflows, the dedicated CF-12 proof, and independent review are terminal and clean. Final task/convergence evidence will be recorded only after immutable run/artifact identities exist.